-
Notifications
You must be signed in to change notification settings - Fork 276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BUG: don't colorize pixels that are not completely within the data domain in pixelize_cylinder routine #3818
BUG: don't colorize pixels that are not completely within the data domain in pixelize_cylinder routine #3818
Conversation
9ab7135
to
0df330d
Compare
The failures are expected. This changes the way we approximate a disk with square pixels: on main, it is approached from the outer side, while here it's approached from the inner side. As a result, on a 800x800 image, we're shaving of a thin band from the reference pixelizations. |
0df330d
to
7dea5c9
Compare
Actually it makes more sense to backport since we may otherwise end up with avoidable conflicts with the backport branch when answer tests are bumped. I'm rebasing this on top of the main branch. |
7dea5c9
to
792a56c
Compare
So, I have to think about this a bit more, but what I'm wondering is if we should instead attempt to set up a "dual grid" where the evaluation points are set at the centers of the pixels. Because we fill from left-right, that would mean basically setting up a grid that overlaid it (and so it would be something like size/factor + 1 I think?) rather than the buffer line setting that is done in here. |
I'm confused. Aren't evaluation points already at the centers ? |
…main in pixelize_cylinder routine
792a56c
to
f72e68d
Compare
This PR doesn't need resolution for 4.0.3 , I'd rather take it out of the milestone rather than having it blocking the release. That said, maybe we can resolve it anyway. I'll take another look at the existing code when I have time and see if this can be revised. |
On reflection, I think some of my question is addressed now that I've looked more closely at the arrows in the plot. I was seeing an asymmetry in the distance from the edge, but I think that's an artifact of my perception; the way the arrow heads are pointing makes it look like the top ones are closer to the edge than the bottom ones. I think this is OK. |
yes there's a top/bottom asymmetry in the cylindrical pixelizer, I never could figure out where it came from but it's not new, and not critical either in production, as far as I can tell. |
@matthewturk just to be sure, can I bump answers for this one ? I also need to refresh the logs: @yt-fido test this please |
yup |
@matthewturk right on, this is now ready ! |
@meeseeksdev backport to yt-4.0.x |
…completely within the data domain in pixelize_cylinder routine
PR Summary
This fixes an issue with the cylindrical pixelizer routine discovered in #3817, where pixels that are not completely within the data domain.
Although a bug fix, I don't think this change is really important before #3817 is merged, so I think this can go to the next feature relase and not be backported.
I'll demonstrate the change using the script from #3817
opening as a draft because this branch is based on #3817 for testing purposes